fix(run): return structured conversation history and preserve multi-artifact tool results#2743
fix(run): return structured conversation history and preserve multi-artifact tool results#2743mike-inkeep wants to merge 6 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
The return-type refactor from string to MessageSelect[] is well-structured — separation of fetching/compressing vs. formatting is a good direction, and the multi-artifact grouping fix is correct. Three areas need attention before merging: a behavioral inconsistency between getFormattedConversationHistory and the new formatMessagesAsConversationHistory, a createdAt regression in the summary message, and missing test coverage for two new code paths.
| @@ -937,8 +947,13 @@ function formatMessagesAsConversationHistory(messages: any[]): string { | |||
| roleLabel = msg.role || 'system'; | |||
| } | |||
|
|
|||
| return `${roleLabel}: """${reconstructMessageText(msg)}"""`; | |||
| const reconstructedMessage = reconstructMessageText(msg); | |||
| if (!reconstructedMessage) { | |||
| return null; | |||
| } | |||
| return `${roleLabel}: """${reconstructedMessage}"""`; | |||
| }) | |||
| .filter((line): line is string => line !== null) | |||
| .join('\n'); | |||
|
|
|||
| return `<conversation_history>\n${formattedHistory}\n</conversation_history>\n`; | |||
There was a problem hiding this comment.
The newly exported formatMessagesAsConversationHistory differs from the inline formatting in getFormattedConversationHistory (around line 376) in two meaningful ways:
- This function uses
reconstructMessageText(msg)(handles multi-part content with artifact refs), while the other usesmsg.content.textdirectly — silently dropping artifact reference tags from multi-part messages. - This function filters out messages with empty reconstructed text, while the other includes them (producing
role: """"""entries).
Since getFormattedConversationHistory is still actively called from AgentSession.ts, these inconsistencies produce different conversation history formats depending on the code path. Consider refactoring getFormattedConversationHistory to delegate to this exported function.
| const referenceMessage = messageHistory[0]; | ||
| const summaryMessage: MessageSelect = { | ||
| id: `summary-${getConversationId()}`, | ||
| tenantId: referenceMessage.tenantId, | ||
| projectId: referenceMessage.projectId, | ||
| conversationId: referenceMessage.conversationId, | ||
| role: 'system', | ||
| fromSubAgentId: null, | ||
| toSubAgentId: null, | ||
| fromExternalAgentId: null, | ||
| toExternalAgentId: null, | ||
| fromTeamAgentId: null, | ||
| toTeamAgentId: null, | ||
| content: { | ||
| text: `[Previous conversation history truncated - ${i + 1} earlier messages]`, | ||
| }, | ||
| visibility: 'system', | ||
| messageType: 'chat', | ||
| createdAt: messageHistory[0].createdAt, | ||
| taskId: null, | ||
| parentMessageId: null, | ||
| a2aTaskId: null, | ||
| a2aSessionId: null, | ||
| metadata: null, | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), |
There was a problem hiding this comment.
createdAt changed from messageHistory[0].createdAt to new Date().toISOString(). The summary message now has a timestamp after all the messages it summarizes, rather than being anchored to the start of the conversation. The array ordering is still correct (it's unshifted), but if any downstream code sorts or filters by createdAt, the summary would sort to the end. The previous behavior was more semantically correct — consider reverting to referenceMessage.createdAt.
| const result = await getConversationHistoryWithCompression(baseParams); | ||
| const toolResult = result.find((msg) => msg.messageType === 'tool-result'); | ||
| const toolResultText = toolResult?.content?.text ?? ''; | ||
|
|
||
| expect(toolResultText).toContain('id: art-1'); | ||
| expect(toolResultText).toContain('id: art-2'); |
There was a problem hiding this comment.
This test verifies both artifact IDs are present but doesn't assert the \n join between them. If the join separator were accidentally changed to empty string or space, this test would still pass. Consider:
expect(toolResultText).toMatch(/id: art-1[\s\S]*\n[\s\S]*id: art-2/);| return parts | ||
| .map((part: any) => { | ||
| if (part.type === 'text') { | ||
| const partKind = part.kind ?? part.type; |
There was a problem hiding this comment.
The part.kind ?? part.type fallback handles both A2A protocol shape ({ kind: 'text' }) and legacy shape ({ type: 'text' }). The existing reconstructMessageText tests only cover { type: ... } parts — there's no test coverage for the { kind: ... } shape. Add a test case to verify the fallback works.
| const reconstructedMessage = reconstructMessageText(msg); | ||
| if (!reconstructedMessage) { | ||
| return null; | ||
| } | ||
| return `${roleLabel}: """${reconstructedMessage}"""`; | ||
| }) | ||
| .filter((line): line is string => line !== null) |
There was a problem hiding this comment.
formatMessagesAsConversationHistory now filters out messages where reconstructMessageText returns empty string — this is new behavior (previously all messages were formatted). A message with only non-text/non-artifact parts (e.g. file parts) would be silently dropped. This needs a direct test, and it's worth confirming this is the desired behavior for all message types.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
conversations.ts:510Args duplicated for each artifact sharing a toolCallId
💭 Consider (2) 💭
💭 1) conversations.ts:903 Defensive fallback for part.kind ?? part.type lacks documentation
Issue: The change from part.type to part.kind ?? part.type suggests two different part schemas exist, but there's no documentation of when each occurs.
Why: The canonical types use kind. If type is a legacy format, a comment would clarify intent.
Fix: Add inline comment explaining the variants, or log occurrences if type is unexpected.
Refs: utility.ts:91-96 (canonical kind-based schema)
💭 2) conversations.ts:950-956 Behavioral change - empty messages now filtered out
Issue: New implementation filters out messages with empty reconstructed text. Previously these appeared as roleLabel: """""".
Why: This is arguably an improvement but goes beyond the stated refactoring goal.
Fix: Document this as intentional and consider adding a test case.
Inline Comments:
- 💭 Consider:
conversations.ts:903Defensivekind ?? typefallback - 💭 Consider:
conversations.ts:950-956Empty message filtering behavior change
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-structured refactoring that improves type safety by returning MessageSelect[] from getConversationHistoryWithCompression and moving formatting to call sites. The multi-artifact support for shared toolCallId values is a valuable fix. The one Minor issue (args duplication) is a small optimization opportunity, and the Consider items are documentation/testing suggestions rather than blocking concerns.
The PR is ready to merge. Nice work on improving the type boundaries! 🎉
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
conversations.artifact-replacement.test.ts:147 |
Test doesn't verify newline-joining format | Nitpick - the core assertion (both artifacts present) is correct |
conversations.test.ts |
Missing tests for kind property in reconstructMessageText |
Valid but downgraded - defensive code for compatibility, not a bug risk |
conversations.artifact-replacement.test.ts:35 |
Test helper missing MessageSelect fields |
Pre-existing pattern in test fixtures, low impact |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-precision |
3 | 0 | 2 | 0 | 1 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 2 | 0 | 1 | 0 | 3 |
| const artifactParts = [ | ||
| `Artifact: "${artifact.name ?? artifact.artifactId}" (id: ${artifact.artifactId})`, | ||
| ]; | ||
| if (argsStr) artifactParts.push(`args: ${argsStr}`); |
There was a problem hiding this comment.
🟡 Minor: Args duplicated for each artifact sharing a toolCallId
Issue: When multiple artifacts share the same toolCallId, the argsStr (computed once from message metadata at lines 492-495) gets appended to every artifact's reference string inside the map. Since all artifacts for the same tool call came from the same invocation, they have identical toolArgs, resulting in the same args: {...} appearing N times in the output.
Why: This unnecessarily bloats token count in the conversation history. For tool calls that produce multiple artifacts (e.g., multiple images or documents), the args string could be repeated 2-10x.
Fix: Consider moving args to a single prefix line before the artifact list:
const argsLine = argsStr ? `Tool call args: ${argsStr}\n` : '';
const artifactRefs = relatedArtifacts.map((artifact) => {
// ... build artifact-specific parts WITHOUT args
});
return {
...msg,
content: { text: argsLine + artifactRefs.join('\n') },
};Refs:
| return parts | ||
| .map((part: any) => { | ||
| if (part.type === 'text') { | ||
| const partKind = part.kind ?? part.type; |
There was a problem hiding this comment.
💭 Consider: Defensive fallback for part.kind ?? part.type
Issue: This fallback suggests there are two different part schemas in play: the canonical MessageContent.parts type (which uses kind per the schema definitions) and some other format that uses type. The fallback is reasonable defensive code, but there's no documentation explaining when each variant occurs.
Why: Without clarity on the source of type-keyed parts, this could mask upstream inconsistencies rather than solve a real polymorphism need. The canonical types in utility.ts and a2a.ts both use kind.
Fix: If both are legitimate variants (e.g., legacy data or external formats), add a brief inline comment:
// Support both 'kind' (canonical schema) and 'type' (legacy/external format)
const partKind = part.kind ?? part.type;If type is never expected, consider logging when encountered to track occurrences.
| const reconstructedMessage = reconstructMessageText(msg); | ||
| if (!reconstructedMessage) { | ||
| return null; | ||
| } | ||
| return `${roleLabel}: """${reconstructedMessage}"""`; | ||
| }) | ||
| .filter((line): line is string => line !== null) |
There was a problem hiding this comment.
💭 Consider: Behavioral change - empty messages now filtered out
Issue: The new implementation filters out messages with empty reconstructed text (returning null then filtering). The previous implementation would include these as roleLabel: """""". This is arguably an improvement, but goes beyond the stated refactoring goal.
Why: This could affect downstream consumers that rely on message boundaries (e.g., compression logic counting messages) or debugging traces. If intentional, it's worth documenting.
Fix: If intentional, consider adding a test case validating the filtering behavior and noting this in the PR description as a deliberate improvement.
There was a problem hiding this comment.
@tim-inkeep does this seem reasonable? It's now filtering out empty messages, whereas before it would sometimes have messages like user: "". I did this because a user a submitting an image without text would sometimes confuse the model, which only saw the empty user text and replied with something like "how can I help you?"
|
TL;DR — Shifts Key changes
Summary ｜ 9 files ｜ 5 commits ｜ base: Return structured messages instead of pre-formatted strings
This decouples history retrieval from serialization, letting future consumers of the history data (e.g. token-counting, streaming, or alternate formats) operate on structured messages without re-parsing a string. The summary placeholder created by
Multi-artifact grouping per
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: Changes since last review (
8eea5bf88a06). The delta consists of a merge commit bringing in PDF attachment support fromorigin/main(PR #2709).
Delta Review Summary
Files with delta changes:
conversation-history.ts: AddedmapFileToAiSdkContentParthelper for PDF/image handling, renamedimageParts→filePartsconversations.ts: UpdatedcreateMessageAPI to use{ scopes, data }structureAgent.test.ts: Added PDF test + mock path rename
Findings: No new issues meeting the severity/confidence thresholds were identified in the delta changes. The PDF support code is well-structured with proper type handling, defensive null returns for unsupported file types, and appropriate warning logging.
🕐 Pending Recommendations (5)
Prior review feedback that remains applicable to the original PR changes:
- 🟡
conversations.ts:510Args duplicated for each artifact sharing a toolCallId - 💭
conversations.ts:903Defensivekind ?? typefallback lacks documentation - 💭
conversations.ts:950-956Empty messages now filtered out (behavioral change) - 🟠
runtime/conversations.ts:268createdAtregression in summary message (pullfrog) - 🟠
conversations.ts:961Behavioral inconsistency between formatting functions (pullfrog)
✅ APPROVE
Summary: The delta changes from the merge commit are clean. PDF attachment support is correctly integrated with the existing file handling infrastructure. Prior review feedback from the first review run remains applicable to the original PR changes — see Pending Recommendations above. 🎉
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 3 | 0 | 0 | 0 | 0 | 0 | 3 |
Note: Test coverage findings (unsupported file types, filename metadata, URI path) were assessed as MEDIUM/LOW confidence and discarded per review criteria.
Ito Test Report ❌7 test cases ran. 1 failed, 6 passed. Overall, the unified run executed 7 test cases with 6 passes and 1 failure (plus several verification runs with no executable cases), indicating generally solid behavior with one confirmed production defect. Key findings were that auth setup and run-domain gates behaved correctly (web_client app creation returned 201, PoW was required and successfully minted anon tokens when solved, and unauthorized origins were blocked with 403), while chat edge checks for 10 parallel requests and a ~50k prompt were stable, but first-turn requests incorrectly inject an empty <conversation_history> wrapper (medium severity, likely introduced by this PR) causing no-history semantics drift. ❌ Failed (1)
🟠 Empty conversation history wrapper injected on first turn
Relevant code:
if (!messagesToFormat.length) {
return [];
}
export function formatMessagesAsConversationHistory(messages: MessageSelect[]): string {
const formattedHistory = messages
.map((msg: MessageSelect) => {
// ...role formatting...
const reconstructedMessage = reconstructMessageText(msg);
if (!reconstructedMessage) {
return null;
}
return `${roleLabel}: """${reconstructedMessage}"""`;
})
.filter((line): line is string => line !== null)
.join('\n');
return `<conversation_history>\n${formattedHistory}\n</conversation_history>\n`;
}
if (conversationHistory.trim() !== '') {
messages.push({ role: 'user', content: conversationHistory });
}✅ Passed (6)
Commit: Tell us how we did: Give Ito Feedback |
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
Re-review scope: Changes since last review (
a96c94951ea5). The delta includes 6 commits with improvements that address prior feedback.
Delta Review Summary
Prior Feedback Addressed in This Delta:
- ✅ Args duplication fixed —
argsStris now output once as a prefixargsLine, not per-artifact - ✅
createdAtregression fixed — Summary message now usesreferenceMessage.createdAtinstead ofnew Date() - ✅ Type safety improved —
toolCallIdnow hastypeof === 'string'guard;reconstructMessageTextproperly typed - ✅ Test coverage for
kind ?? type— Added test for legacypart.typeshape fallback
Delta Code Quality:
The delta changes are clean improvements that tighten types and fix prior issues. The getResponsePartKind helper in executionHandler.ts nicely encapsulates the kind ?? type fallback pattern.
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
conversations.ts:901Empty array produces non-empty wrapper (Ito test failure root cause)
💭 Consider (2) 💭
💭 1) conversations.test.ts Missing test for content.text fallback when parts yield empty
Issue: The implementation at reconstructMessageText (line 898) returns fromParts || textFallback, but there's no test verifying the fallback path when parts exist but produce empty output.
Why: This behavioral change was noted in prior reviews but lacks explicit test coverage documenting the contract.
Fix: Add test: it('falls back to content.text when parts yield empty') with fixture { content: { text: 'fallback', parts: [{ kind: 'text' }] } }.
💭 2) conversations.test.ts Missing test for mixed kind/type parts in same message
Issue: No test verifies behavior when a message contains BOTH legacy type and modern kind parts.
Why: Edge case during data migration; implementation handles it per-part but it's untested.
Fix: Add test with mixed fixture: parts: [{ kind: 'text', text: 'a' }, { type: 'text', text: 'b' }].
🕐 Pending Recommendations (4)
Prior review feedback that may still apply:
- 💭
conversations.ts:879Defensivekind ?? typefallback — consider adding inline comment explaining the two schemas - 💭
conversations.ts:929-934Empty message filtering behavior change — document as intentional (relates to mike-inkeep's question to tim-inkeep) - 🟠
conversations.ts:937Behavioral inconsistency betweengetFormattedConversationHistoryandformatMessagesAsConversationHistory(pullfrog) — now unified, but empty array handling differs - 🟠
conversations.artifact-replacement.test.ts:147Test doesn't verify newline join between artifacts (pullfrog) — now includes/\]\s*\n\n\[/assertion
🚫 REQUEST CHANGES
Summary: The delta changes are solid improvements that address prior feedback. However, there's one Major issue that needs fixing before merge: formatMessagesAsConversationHistory([]) returns a non-empty wrapper string instead of empty string, causing the Ito test failure where first-turn prompts incorrectly include an empty <conversation_history> block. This is a simple fix (early return for empty array).
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
executionHandler.ts:37 |
getResponsePartKind lacks direct unit test |
Low priority - trivial helper, integration coverage sufficient |
utility.ts:119 |
compressionType addition undocumented |
Additive type change, no behavioral impact |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
4 | 0 | 2 | 0 | 0 | 0 | 2 |
| Orchestrator | 1 | 1 | 0 | 0 | 1 | 0 | 0 |
| Total | 5 | 1 | 2 | 0 | 1 | 0 | 2 |
Note: The Major finding was identified by cross-referencing the Ito test failure with the code path analysis.
| } | ||
|
|
||
| function formatMessagesAsConversationHistory(messages: any[]): string { | ||
| export function formatMessagesAsConversationHistory(messages: MessageSelect[]): string { |
There was a problem hiding this comment.
🟠 MAJOR: Empty array produces non-empty wrapper (Ito test failure root cause)
Issue: When messages is empty, this function returns <conversation_history>\n\n</conversation_history>\n instead of an empty string. This wrapper passes the trim() !== '' check in buildInitialMessages, causing an empty history wrapper to be injected into first-turn prompts.
Why: This was flagged by the Ito test suite: "First-turn requests can still inject an empty <conversation_history> wrapper when prior history is empty." This adds noise to first-turn prompts and can cause behavioral drift in edge cases that depend on no-history semantics.
Fix: Return empty string for empty input:
export function formatMessagesAsConversationHistory(messages: MessageSelect[]): string {
if (messages.length === 0) {
return '';
}
// ... rest of function
}Refs:
- Ito test report — documents the failure
- buildInitialMessages:124 — the check that passes erroneously
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: Changes since last review (
37a910e7169a). The delta includes 2 commits with fixes that address the blocking issue from the prior review.
Delta Review Summary
Prior Blocking Issue Resolved:
- ✅ Empty wrapper injection FIXED —
formatMessagesAsConversationHistory([])now returns''instead of<conversation_history>\n\n</conversation_history>\n. Two guards added: early return for empty array (line 903-905) and check for emptyformattedHistoryafter filtering (lines 942-944). This directly addresses the Ito test failure.
Additional Improvements in Delta:
- ✅ Inline comment added — Line 879-880 now documents the
kind ?? typefallback: "CanonicalMessageContent.partsusekind; older persisted rows and some external payloads might still usetype." - ✅ Test coverage added — New
describe('formatMessagesAsConversationHistory')block with 3 tests covering empty array, all-empty-reconstruction, and normal wrapping behavior.
Code Quality:
The delta changes are minimal and focused. The fix is correctly placed — both the early return and the post-filter check are necessary to handle (1) empty input and (2) input that produces no reconstructable text.
🕐 Pending Recommendations (2)
Lower-priority items from prior reviews that remain applicable but are non-blocking:
- 💭
conversations.test.tsMissing test forcontent.textfallback when parts exist but yield empty — would document thefromParts || textFallbackcontract - 💭
conversations.test.tsMissing test for mixedkind/typeparts in same message — edge case during data migration
✅ APPROVE
Summary: The delta resolves the blocking Ito test failure. The empty-wrapper injection bug is fixed with proper guards at both the array-level and post-reconstruction-filtering levels. The implementation is clean, the fix is well-tested, and the inline documentation for the kind ?? type fallback addresses prior feedback. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
2 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: Delta was small enough to review directly without dispatching sub-reviewers.
mike-inkeep
changed the title
mike-inkeep
changed the title
Ito Test Report ✅15 test cases ran. 15 passed. Across the unified QA run, 15 executed test cases passed with 0 failures, and two additional sessions reported no includable verification results. The most important confirmed behaviors were stable conversation continuity and persistence across /run/api/chat and OpenAI-compatible /run/v1/chat/completions (including SSE [DONE]), clean first-turn handling without conversation-history leakage, correct and safe history/tool-result formatting (artifact substitution, no-artifact preservation, truncation limits, and wrapper-text injection resistance), robust non-500 handling for malformed payloads and header abuse, resilience through refresh/navigation/mobile/abort/deep-link flows without duplication or corruption, and cross-context authorization isolation with no secret leakage. ✅ Passed (15)
Commit: Tell us how we did: Give Ito Feedback |
| const reconstructedMessage = reconstructMessageText(msg); | ||
| if (!reconstructedMessage) { | ||
| return null; | ||
| } | ||
| return `${roleLabel}: """${reconstructedMessage}"""`; | ||
| }) | ||
| .filter((line): line is string => line !== null) |
|
@mike-inkeep I would just want to make sure that all functionality still works changing to parts, but i think it will |
2 participants
Summary
getConversationHistoryWithCompressionnow returnsMessageSelect[]instead of a pre-formatted string. Callers that need prompt text use the sharedformatMessagesAsConversationHistoryhelper (wired frombuildConversationHistory).toolCallId: all matching artifacts are inlined as compact references, with a singleTool call args:line when present.content.partshandling treatskindand legacytypeas equivalent inreconstructMessageTextand when persisting streamed assistant turns inExecutionHandler.getConversationHistory/ context-window truncation are typed asMessageSelect[]; synthetic truncation summary rows are fully shapedMessageSelectrecords.MessageMetadatagains optionalcompressionType.Key decisions
MessageSelect[]; formatting to<conversation_history>XML is explicit and reusable (formatMessagesAsConversationHistory) instead of being baked intogetConversationHistoryWithCompression.toolCallIdmaps to an array of artifacts so none are dropped when the ledger has multiple rows for the same id.Manual QA
No manual QA performed.
Test plan
Agent.test,conversations.test,conversations.artifact-replacement.test,ledgerArtifacts.test